chore: improve e2e signalling test tooling#1998
Conversation
|
size-limit report 📦
|
1egoman
left a comment
There was a problem hiding this comment.
Generally makes sense to me!
Just wanted to confirm one thing to make sure I am reading this right - running npm test wouldn't cause the e2e tests to run by default correct? You'd need to explictly run test:e2e and set the relevant LK_ prefixed env vars to the server environment?
| websocketTimeout: 5_000, | ||
| }); | ||
|
|
||
| const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)); |
There was a problem hiding this comment.
nitpick: could this be imported from here instead?
| it('stays connected past the ping timeout while the server pongs', async () => { | ||
| const onClose = vi.fn(); | ||
| await join('happy'); | ||
| client.onClose = onClose; | ||
| // Server ping timeout is 3s; a healthy pong loop must keep us alive. | ||
| await sleep(4_000); | ||
| expect(onClose).not.toHaveBeenCalled(); | ||
| expect(client.currentState).toBe(SignalConnectionState.CONNECTED); | ||
| }); |
There was a problem hiding this comment.
thought: I added this subscribeToEvents test helper here when building data tracks (and I've also used it in the data streams v2 pr). I found it really nice for testing classes which emit events rather than manually registering / unregistering vi.fn() type mocks.
I realize SignalClient isn't an EventEmitter today but that helper looks like it could maybe be useful here as well. If you agree, maybe it should be converted to an EventEmitter and used here too.
If you want to invest in this subscribeToEvents helper more, it might be worth also adding something in the AGENTS.md file about it.
There was a problem hiding this comment.
I'll leave converting SignalClient to an EventEmitter for a potential follow up
| const binary = join(serverDir, 'bin', 'test-server'); | ||
| if (!process.env.LK_E2E_SKIP_BUILD) { | ||
| try { | ||
| execFileSync('go', ['build', '-o', 'bin/test-server', './cmd/test-server'], { |
There was a problem hiding this comment.
question(non-blocking): is this guaranteed to always be the right command, or would it be better to call out to some sort build tool (maybe mage? Or maybe a bespoke script?) in the livekit/livekit repo?
There was a problem hiding this comment.
Still am somewhat curious here, is there a more robust way to handle this?
There was a problem hiding this comment.
it's the command that is also used in the server repo for the test server.
note that this is only ever used for local tests against custom server changes if we're not pulling in the existing docker image (which is what the CI does).
| ttlSeconds?: number; | ||
| } | ||
|
|
||
| export async function createToken(opts: TokenOptions = {}): Promise<string> { |
There was a problem hiding this comment.
question(non-blocking): would it be a good idea to pull in the node server sdk as a dev dependency to generate this token? Or would that be nonviable since you need to add the custom lk.mock attribute?
I mostly bring it up because this could potentially silently break if new fields were added to tokens that should be included by default in newly generated ones. Relying on the existing library should make that a non issue.
| browser: { | ||
| enabled: true, | ||
| provider: playwright(), | ||
| headless: true, | ||
| instances: [{ browser: 'chromium' }], | ||
| }, |
There was a problem hiding this comment.
thought: Is there benefit to running this across multiple browsers? Guessing not for signaling, but maybe there could be for the follow up peer connection management state machine work?
| - name: Run signal e2e tests | ||
| run: pnpm test:e2e 2>&1 | tee e2e.log | ||
| env: | ||
| # Connect to the test-server service above (validate + WS on 9999). | ||
| LK_TEST_SERVER_URL: ws://127.0.0.1:9999 |
There was a problem hiding this comment.
question: Just curious, how stable have these been so far in ci?
There was a problem hiding this comment.
very stable so far!
| const binary = join(serverDir, 'bin', 'test-server'); | ||
| if (!process.env.LK_E2E_SKIP_BUILD) { | ||
| try { | ||
| execFileSync('go', ['build', '-o', 'bin/test-server', './cmd/test-server'], { |
There was a problem hiding this comment.
Still am somewhat curious here, is there a more robust way to handle this?
client side for livekit/livekit#4653